backport: merge bitcoin#24460, #27672, #27766, #28065, #30228, partial bitcoin#28349, #28579 (drop c++17 support) #6380
Conversation
…as c0b716f2 f18e839 build: drop symlinks in immer subtree (Kittywhiskers Van Gogh) d761111 build: fix gitian builds (Kittywhiskers Van Gogh) a9f46b3 Squashed 'src/immer/' changes from 9cb6a5a845..5875f7739a (Kittywhiskers Van Gogh) e4ee302 revert: fix gitian builds (Kittywhiskers Van Gogh) fb00300 revert: drop symlinks in immer subtree (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6380 #6380 will be using `std::ranges` as a replacement for [dash#4622](#4622) and currently, it will fail to compile due to the current version of `immer` (last updated two years ago in #4911) not meeting constraints ([source](https://en.cppreference.com/w/cpp/language/constraints)) set by `std::ranges` (see below). ``` ./evo/deterministicmns.h:243:16: error: no matching function for call to object of type 'const __count_if_fn' return ranges::count_if(mnMap, [](const auto& p) { return IsMNValid(*p.second); }); ^~~~~~~~~~~~~~~~ /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/ranges_algo.h:331:7: note: candidate template ignored: substitution failure [with _Range = const MnMap &, _Proj = identity, _Pred = (lambda at ./evo/deterministicmns.h:243:40)]: constraints not satisfied for alias template 'range_difference_t' [with _Range = const immer::map<uint256, std::shared_ptr<const CDeterministicMN>, CDeterministicMNList::ImmerHasher> &] operator()(_Range&& __r, _Pred __pred, _Proj __proj = {}) const ^ /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/ranges_algo.h:316:7: note: candidate function template not viable: requires at least 3 arguments, but 2 were provided operator()(_Iter __first, _Sent __last, ^ ``` This has been resolved by updating `immer` to the latest available release, [v0.8.1](https://github.com/arximboldi/immer/releases/tag/v0.8.1). Expected subtree hash **without changes** are `a5ded361aec714bc74149909c5e1984c10ab132c4c539c63fe57362dccd95556` (see [here](#6323 (review)) for verification instructions). ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code **(note: N/A)** - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK f18e839 UdjinM6: utACK f18e839 Tree-SHA512: 99d1b577eb4cf3fcc3ebfd28f19006700f085d23ccdabe802a2aa5844e4b39314347b0c0e49a352c5fc034d6584a0109edf08fa3c0846514967f1fb16e7f127a
kwvg
changed the title
|
This pull request has conflicts, please rebase. |
…oble`), merge bitcoin#29985 1599cc6 fix: suppress `float-cast-overflow` UBSan error from `qRound(double)` (Kittywhiskers Van Gogh) a57d128 ci: drop i386 conditional altogether, we don't need `wine32` anymore (Kittywhiskers Van Gogh) b862411 ci: drop conflicting `g++-multilib` package and related workaround (Kittywhiskers Van Gogh) 7b1caa9 fix: remove now-invalid package entry (Kittywhiskers Van Gogh) 9f9e965 fix: use default non-root user `ubuntu` introduced in Ubuntu 22.10 (Kittywhiskers Van Gogh) 9b4c95e ci: update containers and CI to use Ubuntu 24.04 LTS (`noble`) (Kittywhiskers Van Gogh) 8aec25b merge bitcoin#29985: Fix build of Qt for 32-bit platforms with recent glibc (Kittywhiskers Van Gogh) Pull request description: ## Motivation Since [dash#6389](#6389), the minimum supported version of GCC has been 11.1 or later. Our current base, Ubuntu 22.04 LTS (`jammy`) ships with GCC 11.2 ([source](https://packages.ubuntu.com/jammy/gcc)). The cross-compilation package for `arm-linux-gnueabihf` is GCC 11.2 as well ([source](https://packages.ubuntu.com/jammy/gcc-arm-linux-gnueabihf)). Unfortunately, this isn't the case for `mingw-w64-x86-64`, which ships with GCC 10.3 ([source](https://packages.ubuntu.com/jammy/gcc-mingw-w64-x86-64-posix)). So far, an workaround was utilized to allow GCC 10.3 to pass muster upstream through bitcoin#28379 ([source](bitcoin@fa67f09#diff-0baeabda402ee522682c25cef25a248ff6d9e2904f6d66f93f6babf55b576675L986)). Dash Core's enablement of C++20 experimental support in [dash#4600](#4600) was relatively even more permissive ([source](https://github.com/dashpay/dash/pull/4600/files#diff-0baeabda402ee522682c25cef25a248ff6d9e2904f6d66f93f6babf55b576675R168)). Though since then, enforcement of those newer minimum requirements through backports like [bitcoin#30228](bitcoin#30228) are frustrated by the version of GCC shipped for Windows cross-compilation. This can be resolved by bumping the image to the next available LTS release, Ubuntu 24.04 (`noble`), which ships with GCC 13.2 natively ([source](https://packages.ubuntu.com/noble/gcc)), for ARM Linux ([source](https://packages.ubuntu.com/noble/gcc-arm-linux-gnueabihf)) and AMD64 Windows ([source](https://packages.ubuntu.com/noble/gcc-mingw-w64-x86-64-posix)), moreover, it is acknowledged as a _de facto_ lowest supported distro needed for Windows cross compilation by [bitcoin#30580](bitcoin#30580 (comment)). Skipping the enforcement of newer minimum requirements is inadvisable, primarily because it will eventually conflict with upcoming code changes ([comment](bitcoin#30228 (comment))) like [dash#6378](#6378). ## Additional Information * Dependency for #6380 * A default unprivileged user named `ubuntu` was introduced in Ubuntu 22.10 ([source](https://bugs.launchpad.net/cloud-images/+bug/2005129)) with UID `1000` and GID `1000`. We allow specifying UID and GID to ensure they match with the ownership of directories expected to be mounted to avoid permissions issues (especially on platforms like macOS where the user can have a UID of `501` and GID of `20`). * To retain this behavior, the `ubuntu` user and the `ubuntu` group will have its UID and GID updated. This is a no-op if defaults are selected. * In some cases where it may be undesirable to have the username or home directory deviate from the present name (`dash`), it will be additionally renamed from `ubuntu`. * GitLab is unhappy if the container doesn't have a user named `dash` ([build](https://gitlab.com/dashpay/dash/-/jobs/9082688485#L24)) and it results in a runner failure. * Due to a conflict between `gcc-multilib` and `gcc-arm-linux-gnueabihf`, installation of the latter will result in the uninstallation of the former. This has been documented behavior for a while now ([source](https://bugs.launchpad.net/ubuntu/+source/gcc-defaults/+bug/1300211)) and continues till today (see below). <details> <summary>Error message:</summary> ``` ubuntu@ec46b76eeb2c:/src/dash$ sudo apt install gcc-multilib gcc-arm-linux-gnueabihf Reading package lists... Done Building dependency tree... Done Reading state information... Done gcc-arm-linux-gnueabihf is already the newest version (4:13.2.0-7ubuntu1). gcc-arm-linux-gnueabihf set to manually installed. Some packages could not be installed. This may mean that you have requested an impossible situation or if you are using the unstable distribution that some required packages have not yet been created or been moved out of Incoming. The following information may help to resolve the situation: The following packages have unmet dependencies: gcc-arm-linux-gnueabihf : Depends: gcc-13-arm-linux-gnueabihf (>= 13.2.0-11~) but it is not installable E: Unable to correct problems, you have held broken packages. ``` </details> Since [dash#5372](#5372), we have stopped supporting i686 and dropped support for 32-bit Windows even earlier. There doesn't seem to be much reason to keep `gcc-multilib` around, especially since it _cannot_ be around and has been silently uninstalled for a while now, so the package has now been dropped. * Since Wine 7.0, WoW64 support has been included ([source](https://www.winehq.org/announce/7.0)), which allows for some applications to run without a corresponding `wine32` setup ([source](https://wiki.debian.org/Wine#Step_1:_Enable_multiarch)). Support has improved furthermore in Wine 9.0 ([source](https://gitlab.winehq.org/wine/wine/-/releases/wine-9.0)), which is available in `noble` ([source](https://packages.ubuntu.com/noble/wine64)). This allows us to drop the i386 conditional altogether. * `libncurses5`, while a valid package on `jammy` ([source](https://packages.ubuntu.com/jammy/libncurses5)), is not in any future version, including `noble` ([source](https://packages.ubuntu.com/noble/libncurses5), error page) though `libncurses5-dev` continues to remain a valid package ([source](https://packages.ubuntu.com/noble/libncurses5-dev)) and remains used as a Python dependency ([source](https://github.com/dashpay/dash/blob/1930572b05c53d2d8335bcfc22270871d5b0bf2f/contrib/containers/ci/Dockerfile#L91)). As a result, `libncurses5` has been dropped. * A bump in distro base also causes a bump in glibc version, which causes zlib, a Qt dependency to fail a compile-time check ([build](https://github.com/dashpay/dash/actions/runs/13220255380/job/36904408129?pr=6564#step:6:8050)), this is avoided by backporting [bitcoin#29985](bitcoin#29985). * Due to [QTBUG-133261](https://bugreports.qt.io/browse/QTBUG-133261), UBSan will raise a `float-cast-overflow` originating from `qRound(double)` ([build](https://gitlab.com/dashpay/dash/-/jobs/9083558880#L3132)), as of this writing, a fix is currently underway ([source](https://codereview.qt-project.org/c/qt/qtbase/+/621931)) but it is unclear when it would be available in the next Qt 5.15 revision. As this doesn't seem to be a regression ([source](https://bugreports.qt.io/browse/QTBUG-133261?focusedId=860311&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-860311)) despite being undesirable behavior, we can suppress the alarm. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 1599cc6 UdjinM6: utACK 1599cc6 Tree-SHA512: 92f786e9da1440830057729a5d9cdf6448ef9da8c66e0d4c3b80da2e22dd599353f44a31dcbe0ed103febdf627d2a1a332d72de1c3ec312b6ceb665f4bbcb5d1
… to a run-time setting
excludes: - changes to `script_assets_test_minimizer.cpp` (we don't support taproot)
In the next commit, every build will be a C++20 build, we can safely drop this variant.
excludes - fa02fc0 (depends on bitcoin#19988)
std::clock_cast is a part of C++20, hence the original TODO comment but it's still not implemented in Clang, as of this writing. We can sidestep the cast by not using time_t to begin with.
WalkthroughThe changes update the project’s CI workflows, build configurations, source code, and fuzz testing framework. CI jobs related to the linux64_cxx20 target were removed from GitHub Actions and GitLab CI configurations. The project’s C++ standard was upgraded from C++17 to C++20 across multiple files, including CMakeLists.txt, Makefile, autoconf macros, and linting scripts. Several CI setup scripts and environment files were modified or removed, and inclusion directives were updated with IWYU pragmas where applicable. The fuzz testing targets underwent extensive refactoring, replacing the older FUZZ_TARGET_INIT macro with FUZZ_TARGET, which now explicitly specifies an initialization function via the .init parameter. Additional changes include updates in utility headers to use modern C++20 features (such as ranges) and adjustments in error checking and file timestamp handling in the wallet backup code. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/test/fuzz/process_message.cpp (1)
44-44: Consider avoiding global state
LIMIT_TO_MESSAGE_TYPEis declared as a globalstd::string_view. Using a narrower scope for storing the environment-provided message type (such as withininitialize_process_messageor a dedicated configuration struct) can help avoid global side effects and potential concurrency pitfalls.src/test/fuzz/fuzz.cpp (1)
70-74: Duplicate target names
Relying ontry_emplaceplus an assertion will crash if a duplicate name is registered. Consider a more descriptive error or fallback to handle collisions gracefully in the future.src/util/ranges.h (1)
12-12: Mixed use of ranges
You’re invokingstd::ranges::find_if(ds, fn)but still comparing tostd::end(ds). For full consistency with<ranges>, consider usingstd::ranges::end(ds).- if (it != std::end(ds)) { + if (it != std::ranges::end(ds)) {Also applies to: 14-22
src/test/fuzz/validation_load_mempool.cpp (1)
17-19: Consider encapsulating the global state.The global testing setup pointer could be encapsulated within the initialization function to avoid potential issues with global state in tests.
-namespace { -const TestingSetup* g_setup; -} // namespace void initialize_validation_load_mempool() { static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(); - g_setup = testing_setup.get(); + static const TestingSetup* const setup = testing_setup.get(); + return setup; }depends/packages/qt.mk (1)
46-46: Consider usingc++20instead ofc++2a.While
c++2awas the experimental name for C++20 during development, now that C++20 is standardized, it would be clearer and more consistent to usec++20instead.-$(package)_config_opts += -c++std c++2a +$(package)_config_opts += -c++std c++20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
.github/workflows/build.yml(0 hunks).gitlab-ci.yml(0 hunks)CMakeLists.txt(1 hunks)build-aux/m4/ax_cxx_compile_stdcxx.m4(6 hunks)ci/dash/matrix.sh(0 hunks)ci/test/00_setup_env_native_cxx20.sh(0 hunks)ci/test/00_setup_env_native_fuzz.sh(1 hunks)ci/test/04_install.sh(1 hunks)configure.ac(1 hunks)depends/Makefile(1 hunks)depends/README.md(1 hunks)depends/packages/qt.mk(1 hunks)src/bitcoin-cli.cpp(1 hunks)src/compat/assumptions.h(0 hunks)src/fs.h(1 hunks)src/net.cpp(1 hunks)src/net_processing.cpp(1 hunks)src/test/fuzz/addrman.cpp(3 hunks)src/test/fuzz/banman.cpp(1 hunks)src/test/fuzz/bip324.cpp(1 hunks)src/test/fuzz/block.cpp(1 hunks)src/test/fuzz/coins_view.cpp(1 hunks)src/test/fuzz/connman.cpp(1 hunks)src/test/fuzz/descriptor_parse.cpp(1 hunks)src/test/fuzz/deserialize.cpp(1 hunks)src/test/fuzz/fuzz.cpp(3 hunks)src/test/fuzz/fuzz.h(1 hunks)src/test/fuzz/i2p.cpp(1 hunks)src/test/fuzz/integer.cpp(1 hunks)src/test/fuzz/key.cpp(3 hunks)src/test/fuzz/key_io.cpp(1 hunks)src/test/fuzz/load_external_block_file.cpp(1 hunks)src/test/fuzz/message.cpp(1 hunks)src/test/fuzz/net.cpp(1 hunks)src/test/fuzz/p2p_transport_serialization.cpp(4 hunks)src/test/fuzz/parse_univalue.cpp(1 hunks)src/test/fuzz/policy_estimator.cpp(1 hunks)src/test/fuzz/policy_estimator_io.cpp(1 hunks)src/test/fuzz/pow.cpp(1 hunks)src/test/fuzz/process_message.cpp(3 hunks)src/test/fuzz/process_messages.cpp(1 hunks)src/test/fuzz/rpc.cpp(2 hunks)src/test/fuzz/script.cpp(1 hunks)src/test/fuzz/script_sigcache.cpp(1 hunks)src/test/fuzz/script_sign.cpp(1 hunks)src/test/fuzz/socks5.cpp(1 hunks)src/test/fuzz/system.cpp(1 hunks)src/test/fuzz/torcontrol.cpp(1 hunks)src/test/fuzz/transaction.cpp(1 hunks)src/test/fuzz/tx_pool.cpp(2 hunks)src/test/fuzz/utxo_snapshot.cpp(1 hunks)src/test/fuzz/validation_load_mempool.cpp(1 hunks)src/test/fuzz/versionbits.cpp(1 hunks)src/test/util/setup_common.h(1 hunks)src/util/ranges.h(1 hunks)src/util/trace.h(1 hunks)src/validation.cpp(1 hunks)src/wallet/wallet.cpp(2 hunks)test/lint/lint-cppcheck-dash.sh(1 hunks)
💤 Files with no reviewable changes (5)
- ci/dash/matrix.sh
- ci/test/00_setup_env_native_cxx20.sh
- src/compat/assumptions.h
- .gitlab-ci.yml
- .github/workflows/build.yml
✅ Files skipped from review due to trivial changes (7)
- test/lint/lint-cppcheck-dash.sh
- depends/Makefile
- src/fs.h
- src/test/util/setup_common.h
- src/net_processing.cpp
- src/validation.cpp
- src/bitcoin-cli.cpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/test/fuzz/net.cpp
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/message.cpp
[error] 25-25: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/descriptor_parse.cpp
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/connman.cpp
[error] 32-32: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/key_io.cpp
[error] 22-22: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/torcontrol.cpp
[error] 42-42: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/policy_estimator_io.cpp
[error] 19-19: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/process_messages.cpp
[error] 36-36: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/socks5.cpp
[error] 28-28: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/integer.cpp
[error] 50-50: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/i2p.cpp
[error] 20-20: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/utxo_snapshot.cpp
[error] 27-27: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/script_sign.cpp
[error] 33-33: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/parse_univalue.cpp
[error] 21-21: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/block.cpp
[error] 25-25: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/script_sigcache.cpp
[error] 26-26: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/script.cpp
[error] 36-36: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/pow.cpp
[error] 24-24: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/policy_estimator.cpp
[error] 23-23: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/validation_load_mempool.cpp
[error] 27-27: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/versionbits.cpp
[error] 114-114: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/rpc.cpp
[error] 340-340: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/bip324.cpp
[error] 22-22: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/banman.cpp
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/key.cpp
[error] 36-36: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/tx_pool.cpp
[error] 122-122: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/addrman.cpp
[error] 48-48: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/system.cpp
[error] 30-30: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/process_message.cpp
[error] 64-64: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/p2p_transport_serialization.cpp
[error] 31-31: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/load_external_block_file.cpp
[error] 26-26: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/coins_view.cpp
[error] 47-47: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/transaction.cpp
[error] 28-28: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
🪛 LanguageTool
depends/README.md
[uncategorized] ~111-~111: Loose punctuation mark.
Context: ...sed. Defaults to c11. - CXX_STANDARD: Set the C++ standard version used. Defa...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build container / Build container
- GitHub Check: predict_conflicts
🔇 Additional comments (58)
src/test/fuzz/process_message.cpp (3)
8-8: New headers appear appropriate
These newly introduced headers (<primitives/transaction.h>,<serialize.h>,<span.h>,<sync.h>,<cstdlib>,<string_view>, and<vector>) match the functionalities added or used below. No issues found.Also applies to: 11-12, 14-14, 35-35, 39-40
64-67: FUZZ_TARGET integration
The usage ofFUZZ_TARGET(process_message, .init = initialize_process_message)and its callback function is consistent with the new fuzzing framework’s initialization pattern. The unknown macro warning from Cppcheck is likely a false positive requiring configuration.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 64-64: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
70-71: Check for time dependency
Mocking the time to a fixed value is useful for reproducible fuzz tests. However, be aware of any downstream logic that might inadvertently rely on real-time behavior (e.g., timeouts or time-based checks).src/test/fuzz/fuzz.cpp (6)
17-19: Updated includes
These includes (<cstdio>,<cstdlib>,<cstring>,<iostream>,<utility>) directly correlate with the file operations, environment handling, and standard I/O used here, ensuring proper declarations.Also applies to: 23-23, 27-27
59-62:FuzzTargetstruct usage
Encapsulating both the input function and its options into a single struct contributes to clearer organization and code maintainability compared to more ad-hoc approaches.
64-68:FuzzTargetsstorage
Storing fuzz targets in a staticstd::map<std::string_view, FuzzTarget>is efficient. Ensure the string data referenced bystd::string_viewremains valid (e.g., if it’s not pointing to a temporary).
92-110: Single-exit model
The newly introducedshould_exitvariable effectively centralizes exit flow after printing or writing fuzz targets. If multiple environment variables are set (e.g., both printing and writing), you’ll gather partial output from each before exiting. Verify that this combined behavior is intentional.
113-115: Environment-variable caching
Copyingenv_fuzzintog_copybefore using it ensures consistency in the presence of fuzz-engine rewrites or ephemeral environment memory. Good practice for stable usage.
127-128: Initializing the fuzz target
Assigningg_test_one_inputand calling the registeredopts.init()ensures correct setup. Confirm that any dependencies for the fuzz target are also initialized here, if needed.src/util/ranges.h (1)
9-9: Transition to<ranges>
Incorporating<ranges>(C++20) significantly simplifies operations previously handled by macros or manual implementations. This change increases code clarity and maintainability.src/test/fuzz/descriptor_parse.cpp (1)
18-18: LGTM! Macro update aligns with the new initialization pattern.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith an explicit.initparameter improves code clarity while maintaining the same functionality.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 18-18: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/policy_estimator_io.cpp (1)
19-19: LGTM! Macro update maintains optimization pattern.The change to
FUZZ_TARGETwith explicit initialization preserves the efficient reuse of the staticblock_policy_estimator.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 19-19: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/validation_load_mempool.cpp (1)
27-27: LGTM! Macro update follows the new pattern.The change to
FUZZ_TARGETwith explicit initialization maintains consistency with other fuzz tests.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 27-27: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/key_io.cpp (1)
22-22: LGTM! Macro update preserves comprehensive test coverage.The change to
FUZZ_TARGETwith explicit initialization maintains the thorough key encoding/decoding test coverage.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 22-22: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/fuzz.h (3)
24-27: LGTM! Well-structured options class.The new
FuzzTargetOptionsstruct provides a clean and extensible way to configure fuzz targets. The default values are sensible, and the use of a lambda for the default initialization function is elegant.
29-29: LGTM! Clean function signature.The updated function signature using
FuzzTargetOptionsimproves maintainability by encapsulating related parameters.
31-41: LGTM! Improved macro implementation.The new macro implementation with variadic arguments provides a more flexible way to configure fuzz targets while maintaining backward compatibility.
src/test/fuzz/load_external_block_file.cpp (1)
26-26: LGTM! Updated to new initialization syntax.The fuzz target has been correctly updated to use the new initialization syntax without changing its functionality.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 26-26: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/i2p.cpp (1)
20-20: LGTM! Updated to new initialization syntax.The fuzz target has been correctly updated to use the new initialization syntax without changing its functionality.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 20-20: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/message.cpp (1)
25-25: LGTM! Updated to new initialization syntax.The fuzz target has been correctly updated to use the new initialization syntax without changing its functionality.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 25-25: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/socks5.cpp (1)
28-28: LGTM! Macro updated to new syntax.The change from
FUZZ_TARGET_INITtoFUZZ_TARGET(.init = ...)aligns with the broader initiative to modernize the codebase.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 28-28: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/script_sigcache.cpp (1)
26-26: LGTM! Macro updated to new syntax.The change from
FUZZ_TARGET_INITtoFUZZ_TARGET(.init = ...)maintains consistency with other fuzz test files.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 26-26: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/util/trace.h (1)
16-28: LGTM! Simplified tracing macros.The removal of warning suppression macros (
BITCOIN_DISABLE_WARN_ZERO_VARIADIC_PUSH/POP) and direct use ofDTRACE_PROBEfunctions results in cleaner, more maintainable code. This change is safe as the project now requires C++20, where these warnings are no longer relevant.src/test/fuzz/torcontrol.cpp (1)
42-42: LGTM! Macro updated to new syntax.The change from
FUZZ_TARGET_INITtoFUZZ_TARGET(.init = ...)maintains consistency with other fuzz test files.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 42-42: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/parse_univalue.cpp (1)
21-21: LGTM! Clean upgrade to the new fuzz target syntax.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith named parameter.initis correctly applied, maintaining the same initialization function while adopting the modern C++20 syntax.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 21-21: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/process_messages.cpp (1)
36-36: LGTM! Clean upgrade to the new fuzz target syntax.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith named parameter.initis correctly applied, maintaining the same initialization function while adopting the modern C++20 syntax.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 36-36: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/block.cpp (1)
25-25: LGTM! Clean upgrade to the new fuzz target syntax.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith named parameter.initis correctly applied, maintaining the same initialization function while adopting the modern C++20 syntax.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 25-25: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/net.cpp (1)
43-43: LGTM! Clean upgrade to the new fuzz target syntax.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith named parameter.initis correctly applied, maintaining the same initialization function while adopting the modern C++20 syntax.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/utxo_snapshot.cpp (1)
27-27: LGTM! Clean macro update.The change from
FUZZ_TARGET_INITtoFUZZ_TARGET(.init = ...)improves code clarity by making the initialization more explicit while maintaining the same functionality.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 27-27: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/policy_estimator.cpp (1)
23-23: LGTM! Clean macro update.The change from
FUZZ_TARGET_INITtoFUZZ_TARGET(.init = ...)improves code clarity by making the initialization more explicit while maintaining the same functionality.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 23-23: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/pow.cpp (1)
24-24: LGTM! Clean macro update.The change from
FUZZ_TARGET_INITtoFUZZ_TARGET(.init = ...)improves code clarity by making the initialization more explicit while maintaining the same functionality.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 24-24: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/banman.cpp (1)
43-43: LGTM! Clean macro update.The change from
FUZZ_TARGET_INITtoFUZZ_TARGET(.init = ...)improves code clarity by making the initialization more explicit while maintaining the same functionality.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 43-43: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/bip324.cpp (1)
22-22: LGTM! The fuzz target macro update is consistent with the codebase-wide changes.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith explicit.initparameter maintains the same functionality while following the new pattern.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 22-22: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/system.cpp (1)
30-30: LGTM! The fuzz target macro update aligns with the standardized pattern.The change maintains the same initialization behavior while adopting the new macro syntax.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 30-30: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/connman.cpp (1)
32-32: LGTM! The fuzz target macro update follows the standardized pattern.The change maintains the same initialization behavior while adopting the new macro syntax.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 32-32: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/script.cpp (1)
36-36: LGTM! The fuzz target macro update is consistent with the codebase-wide changes.The change maintains the same initialization behavior while adopting the new macro syntax.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 36-36: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/script_sign.cpp (1)
33-33: LGTM! Improved macro syntax for fuzz target initialization.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith explicit.initparameter enhances code clarity while maintaining the same functionality.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 33-33: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/integer.cpp (1)
50-50: LGTM! Consistent update to fuzz target initialization.The macro update follows the same pattern as other files, improving code clarity.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 50-50: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/deserialize.cpp (1)
48-49: LGTM! Updated macro definition for consistent fuzz target initialization.The change to use
FUZZ_TARGETwith.initparameter in theFUZZ_TARGET_DESERIALIZEmacro definition maintains consistency with other fuzz testing files.src/test/fuzz/key.cpp (1)
36-36: LGTM! Consistent updates to all fuzz target initializations.All three fuzz targets (
key,ellswift_roundtrip, andbip324_ecdh) have been updated to use the newFUZZ_TARGETmacro with explicit.initparameter, maintaining consistency across the codebase.Also applies to: 264-264, 283-283
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 36-36: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/rpc.cpp (2)
340-340: LGTM! Fuzz target initialization updated.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith.initparameter aligns with the broader update across the codebase.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 340-340: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
360-360: LGTM! Using C++20'sstarts_withmethod.The change from
rfindtostarts_withis a good improvement, leveraging C++20's string handling features for more readable and efficient code.src/test/fuzz/addrman.cpp (1)
48-48: LGTM! Fuzz target initializations updated consistently.All three fuzz targets have been updated from
FUZZ_TARGET_INITtoFUZZ_TARGETwith.initparameter, maintaining consistency across the codebase.Also applies to: 235-235, 316-316
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 48-48: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/versionbits.cpp (1)
114-114: LGTM! Fuzz target initialization updated.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith.initparameter maintains consistency with other fuzz testing files.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 114-114: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
configure.ac (1)
81-82: LGTM! Enforcing C++20 standard.The change to require C++20 compiler support is consistent with the PR objectives and properly enforces the standard without GNU extensions.
build-aux/m4/ax_cxx_compile_stdcxx.m4 (1)
13-14: LGTM! C++20 support properly implemented.The changes correctly add C++20 support to the build system:
- Documentation updated to include C++20
- C++20 added to supported versions
- Test body for C++20 added with proper feature test macro support
Also applies to: 55-55, 980-1005
src/test/fuzz/tx_pool.cpp (1)
122-122: LGTM! Updated fuzz target macro syntax.The changes correctly update the fuzz target macro syntax to use the new initialization format while preserving the existing functionality.
Also applies to: 296-296
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 122-122: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/p2p_transport_serialization.cpp (1)
31-31: LGTM! Updated fuzz target macro syntax.The changes correctly update the fuzz target macro syntax to use the new initialization format while preserving the existing functionality for all four fuzz targets.
Also applies to: 373-373, 384-384, 395-395
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 31-31: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/wallet/wallet.cpp (2)
25-25: LGTM!The addition of
reverse_iterator.hheader is appropriate as it's used by the modified code inAutoBackupWalletfunction.
855-884: LGTM! Improved timestamp handling using filesystem types.The changes correctly switch from using
std::time_ttofs::file_time_typefor file timestamps, which is more appropriate for filesystem operations. The new code also uses modern filesystem iteration withfs::directory_iteratorandfs::last_write_time.The changes bring several benefits:
- More accurate file timestamp handling using filesystem-specific types
- Better integration with the filesystem library
- Cleaner and more idiomatic code
src/net.cpp (2)
1117-1126: LGTM! Excellent refactoring of V2ShortIDs.The new implementation is more efficient and idiomatic, using modern C++ features like std::fill and std::copy instead of loops. The constexpr function enables compile-time evaluation, and the code is more maintainable.
1118-1119: Important protocol safety checks.The static assertions ensure protocol compatibility by verifying that:
- Bitcoin IDs stay within their allocated range (0-127)
- Dash IDs stay within their allocated range (128-255)
This prevents potential protocol incompatibilities and message type collisions.
ci/test/00_setup_env_native_fuzz.sh (1)
13-13: LGTM! The added warning flag aligns with C++20 migration.The addition of
-Wno-deprecated-volatileto suppress warnings about deprecated volatile qualifiers is appropriate as part of the C++20 migration, where certain uses of the volatile keyword have been deprecated.CMakeLists.txt (1)
11-11: LGTM! C++ standard updated to C++20.The update to C++20 aligns with the PR objectives. Note that this CMakeLists.txt is a dummy project for CLion IDE support and doesn't affect the actual build process.
ci/test/04_install.sh (1)
88-92: LGTM! Enhanced debugging with qa-assets version info.The added subshell block improves debugging by displaying the commit information of the qa-assets repository. The use of a subshell ensures proper directory handling.
depends/README.md (1)
111-111: LGTM! Updated C++ standard in dependency options.The documentation correctly reflects the upgrade to C++20 as the default C++ standard, consistent with the PR's objective.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~111-~111: Loose punctuation mark.
Context: ...sed. Defaults toc11. -CXX_STANDARD: Set the C++ standard version used. Defa...(UNLIKELY_OPENING_PUNCTUATION)
src/test/fuzz/transaction.cpp (1)
28-28: LGTM! Updated to modern fuzz target syntax.The change from
FUZZ_TARGET_INITtoFUZZ_TARGETwith explicit.initparameter improves code clarity by making the initialization more explicit.🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 28-28: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
src/test/fuzz/coins_view.cpp (1)
47-47: LGTM! Fuzz target initialization updated to use named parameter syntax.The change from
FUZZ_TARGET_INIT(coins_view, initialize_coins_view)toFUZZ_TARGET(coins_view, .init = initialize_coins_view)aligns with the broader pattern of fuzz testing framework updates across the codebase. The new syntax using named parameters improves readability and maintainability.Let's verify this pattern is consistently applied across other fuzz testing files:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for consistent usage of the new FUZZ_TARGET syntax across fuzz testing files. # Search for any remaining old FUZZ_TARGET_INIT usage echo "Checking for old FUZZ_TARGET_INIT usage:" rg "FUZZ_TARGET_INIT" src/test/fuzz/ # Search for new FUZZ_TARGET usage echo -e "\nChecking new FUZZ_TARGET usage:" rg "FUZZ_TARGET\([^)]+\.init\s*=" src/test/fuzz/Length of output: 3986
LGTM! Fuzz target initialization across the fuzz tests is using the new named parameter syntax consistently.
- No usage of the old
FUZZ_TARGET_INITmacro remains in the fuzz testing files.- In particular,
src/test/fuzz/coins_view.cppnow usesFUZZ_TARGET(coins_view, .init = initialize_coins_view)as expected.- Other fuzz testing files are also updated to the new syntax.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 47-47: There is an unknown macro here somewhere. Configuration is required. If FUZZ_TARGET is a macro then please configure it.
(unknownMacro)
| if (const auto val{std::getenv("LIMIT_TO_MESSAGE_TYPE")}) { | ||
| LIMIT_TO_MESSAGE_TYPE = val; | ||
| Assert(std::count(getAllNetMessageTypes().begin(), getAllNetMessageTypes().end(), LIMIT_TO_MESSAGE_TYPE)); // Unknown message type passed |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for invalid environment variable
Currently, this uses an assertion to handle unknown message types, which will abort the fuzzing session if the variable is set incorrectly. As a refinement, consider logging a warning and ignoring the variable to allow fuzzing to proceed.
There was a problem hiding this comment.
GH CI in my repo https://github.com/UdjinM6/dash/actions/runs/13294351944
Guix https://github.com/UdjinM6/dash/actions/runs/13294351921
light ACK c4b3dac
There was a problem hiding this comment.
Currently unable to merge in due to configure error:
checking whether /opt/homebrew/opt/llvm/bin/clang accepts -g... yes
checking for /opt/homebrew/opt/llvm/bin/clang option to enable C++11 features... none needed
checking whether make supports the include directive... yes (GNU style)
checking dependency style of /opt/homebrew/opt/llvm/bin/clang... gcc3
checking whether /opt/homebrew/opt/llvm/bin/clang supports C++20 features with -std=c++20... no
checking whether /opt/homebrew/opt/llvm/bin/clang supports C++20 features with +std=c++20... no
checking whether /opt/homebrew/opt/llvm/bin/clang supports C++20 features with -h std=c++20... no
configure: error: *** A compiler with support for C++20 language features is required.
this is with both system clang16 and brew clang19
Not clear if this is me messing something up, or a true issue
|
@PastaPastaPasta is the issue still present? Unable to reproduce locally. |
|
Was able to successfully build in a fresh clone |
Additional Information
Depends on depends: update 'src/immer' to arximboldi/immer@5875f773 as c0b716f2 #6377
Depends on ci: update containers and CI to use Ubuntu 24.04 LTS (
noble), merge bitcoin#29985 #6564Using an existing namespace as an alias for a namespace part of the
stdnamespace (as done inranges) has been done before, see namespacefs, an alias tostd::filesystem(here).While the comment in
wallet.cppdid mention that there would be a better way to do the cast than in C++17, the method to do so, usingstd::chrono::clock_cast, has still not been implemented in Clang (source). The issue has been sidestepped by not usingstd::time_tat all and instead relying onfs::file_time_type, the type returned byfs::last_write_time.Breaking changes
None expected.
Checklist